-
Notifications
You must be signed in to change notification settings - Fork 37
VNT Part 5: VarNamedTuple as VarInfo #1183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Benchmark Report
Computer InformationBenchmark Results |
|
Darn, that is really good.
Am I right in saying the latter two are only really needed for DefaultContext? Edit: Actually, that's a silly question, if not for DefaultContext we don't even need the metadata field at all. |
|
Also, I'm just eyeing this PR and thinking that it's a prime opportunity to clean up the varinfo interface, especially with the functions that return internal values when they probably shouldn't. |
Yes. I'm first trying to make this work without making huge interface changes, just to make sure this can do everything that is needed to do, but I think interface changes should follow close behind, maybe in the same PR or the same release. They'll be much easier to make once there is only two VarInfo types that need to respect them, namely the new one and Threadsafe. |
|
Looks exciting! Two quick quesitons: would this be suitable to
|
One thing I haven't benchmarked, and maybe should, is type unstable models. There is a possibility that type unstable models will be slower with the new approach, because |
|
One comment here is to carefully consider the requirements of particle Gibbs during reviewing so we have sufficient design prevision for
|
|
I think PG should be fine. I think we aren't really removing things so much as shuffling things around and putting them in the right boxes -- so previously where Libtask had to carry a full varinfo, we would now just make it carry a VNT + accumulator tuple. |
…uru/vnt-for-varinfo
|
New benchmarks after some changes: DetailsWe are now comfortably matching or beating the old typed VarInfo in evaluation benchmarks. We are slower, but in the same ballpark, as For some reason our compilation time (more precisely, the first execution of |
Co-authored-by: Penelope Yong <[email protected]>
| end | ||
|
|
||
| function unflatten!!(vi::VarInfo, vec::AbstractVector) | ||
| function unflatten!!(vi::VarInfo{Linked}, vec::AbstractVector) where {Linked} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re. comment below this: Did you look into the closure boxing thing, or should I just leave it for another time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm testing it out, but so far it hasn't yielded much. I get the feeling the time cost is somewhere else, but I don't yet understand where.
| new_linked = if LinkedLeft == LinkedRight | ||
| LinkedLeft | ||
| else | ||
| nothing | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pathological, but what if one of them is empty but has the opposite link status? Then we should just take the link status from the non-empty one. Alternatively, only determine the new link status by iterating through the values again after merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair points. I've added a comment, because I don't think this is high priority to fix right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's fair, it's just a performance hit rather than correctness.
This is concerning. I should try removing them. I would be surprised if Julia was able to optimise these things itself, because a lot of the |
Co-authored-by: Penelope Yong <[email protected]>
|
I think this is good enough to merge. I've listed the residual issues to check and polish here: #1201. @penelopeysm, are you happy? |
|
🎉 |
I put together a quick sketch of what it would look like to use
VarNamedTupleas aVarInfodirectly. By that I mean having aVarInfotype that is nothing but accumulators plus aVarNamedTuplethat maps eachVarNameto a tuple (or actually a tiny struct, but anyway) of three values: Stored value for this variable, whether it's linked, and what transform should be applied to convert the stored value back to "model space". I'm calling this new VarInfo typeVNTVarInfo(name to be changed later).This isn't finished yet, but the majority of tests pass. There are a lot of failures around edge cases like Cholesky and weird VarNames and such, but for most simple models you can do
and it'll give you the correct result.
unflattenandvi[:]also work.I'll keep working on this, but at this point I wanted to pause to do some benchmarks, see how viable this is. Benchmark code, very similar to #1182, running
evaluate!!on our benchmarking models:Details
Results contrasting the new
VarInfowith both the oldVarInfoand withSimpleVarInfo{NamedTuple}andSimpleVarInfo{OrderedDict}. Some SVI NT results are missing because it couldn't handle theIndexLenses:I think a fair TL;DR is that for both small models and models with
IndexLenses this is many times faster than the oldVarInfo, and not far off fromSimpleVarInfowhenSimpleVarInfois at its fastest (NamedTuples for small models, OrderedDicts forIndexLenses). I would still like to close that gap a bit, I don't know why linking causes such a large slowdown in some cases, I suspect it's because the transform system is geared towards assuming we want to vectorise things, and I've hacked this together quickly to just get it to work.For large models performance is essentially equal, as it should be, because this is about overheads. To fix that, I need to look into using views in some clever way, but that's for later.
I think this is a promising start towards being able to say that all of
VarInfo,SimpleVarInfo, andVarNamedVectorcould be replaced with a direct use ofVarNamedTuple(as opposed to e.g. VNT wrappingVarNamedVector), and it would be pretty close to being a best-of-all-worlds solution, in that it's almost as fast as SVI and has full support for all models.Note that the new VNTVarInfo has no notion of typed and untyped VarInfos. They are all as typed as they can be, which should also help simplify code.
I'll keep working on this tomorrow.